Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ref(flags): underscore featureflags module and identifier #3902

Merged
merged 4 commits into from
Jan 7, 2025
Merged

ref(flags): underscore featureflags module and identifier #3902

merged 4 commits into from
Jan 7, 2025

Conversation

aliu39
Copy link
Member

@aliu39 aliu39 commented Jan 6, 2025

Follow-up to #3860, which hasn't been released yet

This was referenced Jan 6, 2025
Closed
Merged
@codecov Codecov
Copy link

codecov bot commented Jan 6, 2025 •
edited
Loading

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
13784 1 13783 4153
View the top 1 failed tests by shortest run time 
tests.test_transport test_transport_works[threads-False-gzip-0-True-flush-False]
Stack Traces | 2.52s run time 
tests/test_transport.py:187: in test_transport_works
    assert capturing_server.captured
E   AssertionError: assert []
E    +  where [] = <CapturingServer(<class 'tests.test_transport.CapturingServer'>, started 139966323615488)>.captured

To view more test analytics, go to the Test Analytics Dashboard
📢 Thoughts on this report? Let us know!

antonpirker
antonpirker requested changes Jan 7, 2025
View reviewed changes
Copy link
Member

@antonpirker antonpirker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a breaking change and needs a major update for the SDK.

Is this something we MUST have or it is just a nice too have?
I would not want to make this breaking change only for cosmetic reasons.

@antonpirker
Copy link
Member

@aliu39 and @cmanallen the FeatureFlagsIntegration is not documented yet so I guess it is still in beta or some kind of stealth mode, right?
If it is not yet used by a lot of users, we probably can rename it without a major update.

antonpirker
antonpirker approved these changes Jan 7, 2025
View reviewed changes
Copy link
Member

@antonpirker antonpirker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In our feature flag documentation we state that feature flag tracking is a beta feature.

Also FeatureFlagsIntegration is not yet documented at all.

So renaming this is OK in a minor release.

@antonpirker antonpirker merged commit bf65ede into master Jan 7, 2025
137 of 138 checks passed
@cmanallen
Copy link
Member

@antonpirker The featureflags modules hasn't been released yet so unless someone is running from master no one has ever used the integration. That's why we felt it was safe. Thanks for merging this! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cmanallen cmanallen cmanallen approved these changes

@antonpirker antonpirker antonpirker approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@aliu39 @antonpirker @cmanallen